Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Sfold #6504

Open
wants to merge 42 commits into
base: main
Choose a base branch
from
Open

Add Sfold #6504

wants to merge 42 commits into from

Conversation

reytakop
Copy link

FOR CONTRIBUTOR:

  • I have read the CONTRIBUTING.md document and this tool is appropriate for the tools-iuc repo.
  • License permits unrestricted use (educational + commercial)
  • This PR adds a new tool or tool collection
  • This PR updates an existing tool or tool collection
  • This PR does something else (explain below)

bgruening
bgruening reviewed Oct 31, 2024
View reviewed changes
Copy link
Member

@bgruening bgruening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a .shed.yml to trigger the tests.

tools/sfold/sfold.xml Outdated Show resolved Hide resolved
@reytakop
Copy link
Author

reytakop commented Nov 8, 2024

The sfold.xml file was edited and completed. I also added a .shed.yml file.

@bgruening
Copy link
Member

sh: line 1: /usr/local/bin/gcg2bp.pl: cannot execute: required file not found

bgruening
bgruening reviewed Nov 18, 2024
View reviewed changes
tools/sfold/macros.xml Outdated Show resolved Hide resolved
@bgruening
Copy link
Member

The file is in the container:

root@0c1333b9f6d8:/# find / -name '*gcg2*'
/usr/local/bin/gcg2bp.pl

Shebang looks also good:

root@0c1333b9f6d8:/# head /usr/local/bin/gcg2bp.pl
#!/usr/bin/env perl

#
# Take GCG connect file as input and convert structure
# to base-pair format
#

```

Maybe it was really just the wrong version number.

@reytakop reytakop closed this Nov 18, 2024
@reytakop reytakop reopened this Nov 18, 2024
@reytakop reytakop closed this Nov 19, 2024
@reytakop
Copy link
Author

The Planemo test was passed.

@reytakop reytakop reopened this Nov 19, 2024
@reytakop reytakop marked this pull request as ready for review November 19, 2024 10:49
bgruening
bgruening reviewed Nov 19, 2024
View reviewed changes
tools/sfold/sfold.xml Outdated Show resolved Hide resolved
tools/sfold/sfold.xml Outdated
<option value="2">Soligo</option>
<option value="3">Both</option>
</param>
<section name="output" title="Output Options">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this section can be removed I think, there is only one option

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review. Why only one option?
Sfold can use two modules, "Soligo", "Sirna", or both. It can also use none.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its only one select parameter. I'm not sure its worthwhile to hide one parameter under a section. Imho it make sense to hide many parameters in a section. Its just an opinion you can also keep it like that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comment. The sfold.xml file was updated based on that.

tools/sfold/sfold.xml Show resolved Hide resolved
@reytakop
Copy link
Author

The sfold.xml file was edited to fix the error related to test 2. 

reytakop and others added 3 commits November 23, 2024 15:03
nilchia
nilchia suggested changes Nov 23, 2024
View reviewed changes
tools/sfold/sfold.xml Outdated Show resolved Hide resolved
nilchia
nilchia suggested changes Nov 23, 2024
View reviewed changes
tools/sfold/sfold.xml Outdated Show resolved Hide resolved
tools/sfold/sfold.xml Outdated Show resolved Hide resolved
tools/sfold/sfold.xml Outdated Show resolved Hide resolved
tools/sfold/sfold.xml Outdated Show resolved Hide resolved
tools/sfold/sfold.xml Outdated Show resolved Hide resolved
tools/sfold/sfold.xml Outdated Show resolved Hide resolved
nilchia
nilchia suggested changes Nov 25, 2024
View reviewed changes
Copy link
Contributor

@nilchia nilchia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO select with multiple="true" without checkbox does not have a good UI/UX.

tools/sfold/sfold.xml Outdated Show resolved Hide resolved
tools/sfold/sfold.xml Outdated Show resolved Hide resolved
tools/sfold/sfold.xml Outdated Show resolved Hide resolved
tools/sfold/sfold.xml Outdated Show resolved Hide resolved
tools/sfold/sfold.xml Outdated Show resolved Hide resolved
tools/sfold/sfold.xml Outdated Show resolved Hide resolved
tools/sfold/sfold.xml Outdated Show resolved Hide resolved
nilchia
nilchia suggested changes Nov 26, 2024
View reviewed changes
tools/sfold/sfold.xml Outdated Show resolved Hide resolved
nilchia
nilchia suggested changes Nov 26, 2024
View reviewed changes
tools/sfold/sfold.xml Outdated Show resolved Hide resolved
tools/sfold/sfold.xml Outdated Show resolved Hide resolved
tools/sfold/sfold.xml Outdated Show resolved Hide resolved
tools/sfold/sfold.xml Outdated Show resolved Hide resolved
tools/sfold/sfold.xml Outdated Show resolved Hide resolved
@nilchia
Copy link
Contributor

nilchia commented Nov 26, 2024

looks good to me. Thanks @reytakop

@nilchia
Copy link
Contributor

nilchia commented Nov 26, 2024

Congratulations on your first contribution to the Galaxy Reyhaneh! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bgruening bgruening bgruening left review comments

@nilchia nilchia nilchia approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@reytakop @bgruening @nilchia